Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setup_environment to truss #1188

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

spal1
Copy link
Collaborator

@spal1 spal1 commented Oct 11, 2024

🚀 What

This PR adds a new setup_environment function that we will call when we notice a change to the environment a truss is running in. This environment refers to the release environment, ex: production or staging. In order to notice these changes, we will need to watch for updates to a particular environment file mounted on the b10_dynamic_config directory.

Going to add support for setup_environment for chainlets in a separate PR: #1192

💻 How

  • From the truss server on_startup, we call a `setup_polling_for_environment_updates() function that will create the asyncio task that will run the file watcher
  • Inside the task, we will wait for the ModelWrapper._status to be READY, indicating that the load() was successfully completed, and then will start actually watching the /etc/b10_dynamic_config/environment file
  • Whenever the modification time of the file changes, we will use the dynamic_config_resolver to access the contents of the /etc/b10_dynamic_config/environment file and call the user-defined setup_environment function with the contents

🔬 Testing

TODO:

  • call user-defined setup_environment from load_impl - this is to enable setting up the environment before user-defined load is called
  • Add integration tests to truss/tests/test_model_inference.py
  • Add unit test for new async functions in dynamic_config_resolver
  • End to end test with the configmap patching flow in dev

Loom with e2e test: https://www.loom.com/share/cbe8e03a7d97434f99f973eaceb33c59?sid=ca51f099-fd45-4b39-a7bf-366d7d01356b

Copy link

linear bot commented Oct 11, 2024

truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/test_data/file_watcher_truss/model/model.py Outdated Show resolved Hide resolved
@@ -298,6 +298,12 @@ def _run_docker(gpus: Optional[str] = None):
f"src={str(LocalConfigHandler.bptr_data_resolution_dir_path())}",
"target=/bptr",
],
[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my own edification, what does this help accomplish?

Copy link
Collaborator Author

@spal1 spal1 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we mimic the mounted configmap directory (which is /etc/b10_dynamic_config on model pods) here so we can make changes to /etc/b10_dynamic_config/environment in the integration tests to mimic patches to the environment key in the dynamic_config configmap

@spal1 spal1 force-pushed the samiksha/bt-12427-add-support-for-setup_environment-to-truss branch 2 times, most recently from 59a4aa9 to 3bae49a Compare October 16, 2024 00:41
@spal1 spal1 changed the title Add setup_environment Add setup_environment to truss Oct 16, 2024
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved

# Skip polling if no setup_environment implementation provided
if not self.model_descriptor.setup_environment:
self._logger.info("No model.setup_environment definition provided")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to log here, if the user didn't provide a setup_environment method, I don't think this is the right way to communicate this to them.

On the other hand, if they did provide a method, it might be good to log something indicating that we will poll for changes to the env.

truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
truss/templates/server/model_wrapper.py Show resolved Hide resolved
truss/templates/shared/dynamic_config_resolver.py Outdated Show resolved Hide resolved
truss/tests/test_model_inference.py Outdated Show resolved Hide resolved
@spal1 spal1 marked this pull request as ready for review October 16, 2024 21:49
Copy link
Collaborator

@squidarth squidarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good, would you mind making this a context builder and testing on dev/staging? Ideally we can run truss-examples on it (I can walk you through how to do that)

@@ -96,6 +97,7 @@ pytest = "7.2.0"
pytest-cov = "^3.0.0"
types-PyYAML = "^6.0.12.12"
types-setuptools = "^69.0.0.0"
types-aiofiles = "^24.1.0.20240626"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a less specific version here? (maybe "^24.1.0")

truss/templates/server/model_wrapper.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@squidarth squidarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment, but this looks awesome, amazing work here!!

await self.setup_environment(environment_json)
self._environment = environment_json
except Exception as e:
logging.error(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it shows up nicer if you do:

logging.error("blah blah", exc_info=e)

@spal1 spal1 self-assigned this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants